Skip to content

feat: settings in seed #682

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Jun 21, 2021
Merged

feat: settings in seed #682

merged 9 commits into from
Jun 21, 2021

Conversation

@pierreTklein
Copy link
Member

@pierreTklein pierreTklein commented Nov 11, 2020 •
edited by jamesxu123
Loading

Tickets:

List of changes:

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

  • refactor: move const settings objects into constants file
  • feat: add settings to seed script
  • fix: update settings schema defaults so that they are more sensible

Type of change

  • New feature (non-breaking change which adds functionality)

How has this been tested?

  • Ran npm run seed manually
  • Test B

Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

Questions for code reviewers?

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • Listed change(s) in the Changelog
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have made corresponding changes to the documentation
  • Any dependent changes have been merged and published in downstream modules

@pierreTklein pierreTklein requested a review from logan-r November 11, 2020 00:41
logan-r
logan-r approved these changes Nov 12, 2020
View reviewed changes
Copy link
Member

@logan-r logan-r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

models/settings.model.js
openTime: {
type: Date,
default: 0
default: Date.now() + 2628000000 // One month from now.
Copy link
Member

@logan-r logan-r Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it'd be a good idea to have it default to a day before Date.now()?
Only time I can think of were we should be using defaults is for local environment and applications open is probably most useful state for that?

Copy link
Member Author

@pierreTklein pierreTklein Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set it 1mo from now for production reasons. I wouldn't want the API to be able to accept applications right away.

Copy link
Member Author

@pierreTklein pierreTklein Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other time we use defaults is when you create a new DB for production (which may or may not happen, but it could).

Copy link
Member

@logan-r logan-r Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's fair

@jamesxu123 jamesxu123 linked an issue Jun 21, 2021 that may be closed by this pull request
Incorporate settings object into seed script #678
Closed
@jamesxu123 jamesxu123 temporarily deployed to testing June 21, 2021 04:27 Inactive
jamesxu123
jamesxu123 requested changes Jun 21, 2021
View reviewed changes
tests/util/settings.test.util.js Outdated
Comment on lines 40 to 41
settingConfirmClosed: Constants.Settings.CONFIRM_CLOSED,
settingRemoteHackathon: Constants.Settings.REMOTE_HACKATHON
Copy link
Member

@jamesxu123 jamesxu123 Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this export and just have whatever code referencing this to just import Constants instead to be consistent?

Copy link
Contributor

@chenxuan-zhou chenxuan-zhou Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, since Constants.Settings is not used directly anywhere in this util script.

Copy link
Contributor

@chenxuan-zhou chenxuan-zhou Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamesxu123 Sorry for my previous comment. I re-read settings.test.util.js and saw that Constants.Settings actually is used in lines 8, 14, 25.

I also checked out other *.test.js and *.test.util.js scripts, and it seems like we have previously been exporting objects from the *.test.util.js scripts. However, that was due to the need to create objects using the constants (example).

Since here we use the constants directly, should I just import them into settings.test.js, or would it be better to follow the same structure and keep the export here?

Copy link
Member

@jamesxu123 jamesxu123 Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chenxuan-zhou Let's move it all to Constants. It's more consistent with the rest of the constant vars.

@chenxuan-zhou chenxuan-zhou temporarily deployed to testing June 21, 2021 21:31 Inactive
jamesxu123
jamesxu123 approved these changes Jun 21, 2021
View reviewed changes
Copy link
Member

@jamesxu123 jamesxu123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@jamesxu123 jamesxu123 changed the title Feat/678 settings in seed feat: settings in seed Jun 21, 2021
@jamesxu123 jamesxu123 merged commit ef9f7c7 into dev Jun 21, 2021
@jamesxu123 jamesxu123 deleted the feat/678-settings-in-seed branch June 21, 2021 21:58
logan-r pushed a commit that referenced this pull request Nov 29, 2021
@pierreTklein @chenxuan-zhou
@jamesxu123 @logan-r
feat: settings in seed (#682)
ea39a88 
* refactor: move const settings objects into constants file

* feat: add settings to seed script

* fix: update settings schema defaults so that they are more sensible, and use that in seed

* fix: export and use settings constants

* fix: import setting constants directly in test

Co-authored-by: chenxuan-zhou <46543122+chenxuan-zhou@users.noreply.github.com>
Co-authored-by: James Xu <xujames007@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chenxuan-zhou chenxuan-zhou chenxuan-zhou left review comments

@jamesxu123 jamesxu123 jamesxu123 approved these changes

@logan-r logan-r Awaiting requested review from logan-r

@Anasshahidd21 Anasshahidd21 Awaiting requested review from Anasshahidd21

@meldunn meldunn Awaiting requested review from meldunn

@pengyk pengyk Awaiting requested review from pengyk

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Incorporate settings object into seed script

5 participants

@pierreTklein @logan-r @jamesxu123 @chenxuan-zhou